Fixes text centering causing offsets by some glyphs#31412
Conversation
|
Hey there @@dotMorten! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes text centering issues in FontImageSource rendering on Windows that was causing glyph offsets. The fix updates the positioning calculation to properly center glyphs within the canvas.
- Replaces incorrect DrawBounds-based positioning with proper canvas centering calculation
- Uses LayoutBounds and DefaultFontSize for more accurate text positioning
…ceService.Windows.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@clandrew @shawnhar This is the best I could come up with to fix the text centering issue, but you two might be the best ones to confirm the use of CanvasTextLayout and calculating centering. Would appreciate your input on this. I didn't really find anything else in the layout that gave the values needed to correctly center the symbol. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
🤖 AI Summary📊 Expand Full Review —
|
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #31412 | Replace the previous DrawBounds-based 1px-offset positioning with a canvas-center calculation derived from layout.LayoutBounds and layout.DefaultFontSize. |
⏳ PENDING (Gate) | src/Core/src/ImageSources/FontImageSourceService/FontImageSourceService.Windows.cs |
Original PR |
🚦 Gate — Test Verification
Gate Result: ⚠️ SKIPPED
Platform: android (requested)
Mode: Full Verification
- Tests FAIL without fix: ❌
- Tests PASS with fix: ❌
Reason: PR #31412 does not include UI/device test coverage for issue #30004, and the bug is explicitly Windows-specific while the requested Android platform is unaffected. Existing repository matches for FontImageSource are generic service tests rather than an issue-focused verification path for this glyph-centering bug, so a meaningful fail-without-fix/pass-with-fix gate could not be established in this environment.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix (claude-opus-4.6) |
Use Math.Max(LayoutBounds, DrawBounds) for canvas sizing, then geometrically center DrawBounds via canvas-center minus draw-bounds center. |
1 file | Blocked by Linux host lacking workable Windows targeting packages. | |
| 2 | try-fix (claude-sonnet-4.6) |
Give CanvasTextLayout explicit fontSize x fontSize dimensions and rely on Win2D's built-in center alignment, drawing at origin with no manual offsets. |
1 file | Different from PR math and attempt 1, but still blocked on Linux. | |
| 3 | try-fix (gpt-5.3-codex) |
Use LayoutBounds as the centering frame: size the canvas from LayoutBounds (+padding) and place the layout at canvas center - layout bounds center. |
1 file | Avoids DefaultFontSize heuristics and DrawBounds centering, but blocked on Linux. |
|
| 4 | try-fix (gemini-3-pro-preview) |
Convert the text layout to CanvasGeometry, compute the exact ink bounds, and render via FillGeometry with a translated offset based on geometry bounds. |
1 file | Distinct vector-geometry strategy; blocked on Linux. | |
| 5 | try-fix (claude-opus-4.6, round 2) |
Use CanvasTextLayout.LineMetrics baseline/height data to center typographic metrics instead of visual bounds. |
1 file | Metric-driven strategy; blocked because Windows TFMs are not available from this Linux environment. | |
| 6 | try-fix (gpt-5.3-codex, round 2) |
Two-pass DrawBounds-sized layout: measure first, then create a second layout whose box matches the measured ink bounds and let Win2D center within that box. |
1 file | Different built-in-centering variant; blocked on Linux. | |
| 7 | try-fix (claude-opus-4.6, round 3) |
Render to an offscreen bitmap, scan alpha pixels for visual bounds/centroid, then offset the final draw to center the actual rendered ink. | 1 file | First post-render pixel-analysis strategy; blocked on Linux. | |
| 8 | try-fix (claude-sonnet-4.6, round 3) |
Use the font's CapHeight metric via CanvasFontFace as the vertical centering reference instead of line height or rendered bounds. |
1 file | Cap-height strategy implemented with fallback behavior; blocked on Linux. | |
| 9 | try-fix (gpt-5.3-codex, round 3) |
Shape text to glyph IDs and use per-glyph font-table metrics to compute centering offsets directly from typographic data. | 1 file | Most font-table-driven strategy; blocked on Linux. | |
| PR | PR #31412 | Replace the prior DrawBounds + 1px offset math with canvas center + LayoutBounds + DefaultFontSize / 2. |
1 file | Original PR; Gate was skipped because no applicable automated verification path was available. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Suggested font-metrics / baseline-centered alignment. |
| claude-sonnet-4.6 | 2 | No | NO NEW IDEAS |
| gpt-5.3-codex | 2 | Yes | Suggested two-pass DrawBounds-sized layout using Win2D built-in centering. |
| gemini-3-pro-preview | 2 | No | NO NEW IDEAS |
| claude-opus-4.6 | 3 | Yes | Suggested offscreen render + alpha-bounds centroid centering. |
| claude-sonnet-4.6 | 3 | Yes | Suggested cap-height-based vertical alignment. |
| gpt-5.3-codex | 3 | Yes | Suggested glyph-shaping / per-glyph font metrics centering. |
| gemini-3-pro-preview | 3 | No | NO NEW IDEAS |
Exhausted: Yes
Selected Fix: PR #31412 (tentative) — no alternative candidate could be empirically validated on this Linux host, and none produced a passing verification result that would justify preferring it over the PR.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Windows-only FontImageSource centering bug tied to issue #30004; PR changes one Windows implementation file and no tests. |
| Gate | Requested platform was Android, but the bug is Windows-specific and no issue-focused verification path exists in the PR. | |
| Try-Fix | ✅ COMPLETE | 9 attempts completed, 0 passing, all blocked by Linux-host Windows-targeting limitations. |
| Report | ✅ COMPLETE |
Summary
PR #31412 addresses a real Windows glyph-centering problem in FontImageSource, and the proposed math is plausible. However, this review could not establish fail-without-fix/pass-with-fix coverage, and no empirical winner emerged from the mandatory alternative-fix exploration because the affected code only compiles under Windows-targeted builds while this review environment is Linux.
Root Cause
The bug appears to come from how Windows FontImageSourceService positions text within the generated canvas: the previous implementation sized from DrawBounds and then applied a fixed 1px-offset formula that does not reliably center all glyphs. The PR changes that to a LayoutBounds/DefaultFontSize-based centering formula, but without test coverage or Windows validation here, correctness remains unproven.
Fix Quality
The PR is small and targeted, which is good, but it currently lacks the evidence needed for approval:
- No issue-focused automated test was added for "FontImageSource not center-aligned inside Image control in .NET MAUI" #30004.
- Gate verification could not run meaningfully on the requested Android platform because the defect is Windows-only.
- The repo itself gates Windows target frameworks behind
IsOSPlatform('windows')inDirectory.Build.props, which is why every alternative attempt was blocked on this Linux host rather than producing a comparable pass/fail result.
Given that, the next step should be to add or identify a Windows verification path and validate the PR on a Windows-capable environment before approving.
MauiBot
added
s/agent-changes-requested
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
I think it is good! But could you please try to add a test for this?
Description of Change
The font alignment calculation was wrong, often causing the symbols to be offset when rendering using
FontImageSource.Issues Fixed
Fixes #30004
Below is an example of the adjustment. First column using
<Label />to render the Glyph, second is how MAUI currently renders it, and 3rd column with the fix applied. I applied a background to the two images, to make it clear that the generated image sizes hasn't changed, only the alignment within them.